-
-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moved function overloads from Color.set to rest types #615
Conversation
…arameters<T>` utility type to pick it up correctly
Thanks for the PR! In general, I prefer to keep workarounds as close to the source of the problem as possible. In this case, we're compensating for the shortcomings of the |
Hi @Methuselah96, Thanks for sharing the workarounds. I created a TypeScript playground to show that the solution proposed in this PR is as safe as the current solution and also provides intellisense as function overloads. On top of that |
Yeah, makes sense, I assume we would only be doing this for any overloaded I'm probably okay with this as long as this pattern doesn't need be to used too much, and with the understanding if any downsides emerge down the road that we're likely to go back to using overloads. |
That would make sense, although I'm not aware of other ones. At least this is the one with the largest impact on our user side.
Absolutely. If any downsides emerge, I'm all for reverting. Thanks for considering! I'll mark this ready for review then. |
Thanks! This will get released with r157 at the end of the month. |
cc @CodyJasonBennett This might be useful for |
Thanks for looking out. Yeah, we worked around this already, but I'm happy to review any related work in the future. |
Hey @CodyJasonBennett, sorry, didn't want to blindside you. Is there a blocker for this from the R3F side? I guess this is also beneficial for you. |
No blocker, I've worked around this in pmndrs/react-three-fiber#2931 and pmndrs/react-three-fiber#2932 (the v9 branch may be easier to follow for further inspection). We rely heavily on inference, moreso in v9 also which is strictly for types/internal tests. The linked workarounds from TS triage don't work very well since they can create complex unions that fail from complexity. What you can do alternatively to this PR, while still providing overloads, is follow overloads with a mixed signature that reflects the implementation: set(r: number, g: number, b: number): this;
set(color: ColorRepresentation): this;
set(...args: [color: ColorRepresentation] | [r: number, g: number, b: number]): this;
|
Moved function overloads from Color.set to rest types in order for
Parameters<Color['set']>
to pick it up correctly.Why
Color.set
is implemented as a function with two overloads. The TypeScript utiliity typeParameters<T>
will only return the parameter types of the last overload. Threlte (and other frameworks such as @react-three/fiber) is using this type to infer the possible prop types, for example:What
A possible solution is to implement the types as rest types. This PR is marked as a draft PR for discussion purposes. If this is a viable way to move forward, I would be happy to remove the draft status.
Checklist
master
, next goesdev
)